src/middleware.test.ts (1)

137-150: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add regression coverage for early-return non-account routes.

This test only exercises the generic /api/* branch. Please also cover /?wc-api=am-software-api and updates.wcpos.com/api/*, since those branches can bypass the sanitized header forwarding path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/middleware.test.ts` around lines 137 - 150, The current middleware test
only covers the generic /api/* path, so add regression coverage for the other
early-return non-account routes that may bypass the sanitized header forwarding
path. Extend middleware.test.ts with cases for the root ?wc-api=am-software-api
route and updates.wcpos.com/api/*, using middleware() and the
ACCOUNT_REQUEST_HEADER assertions to verify the client-supplied header is still
stripped from the forwarded NextResponse.next({ request }) headers.

src/lib/impersonation.test.ts (1)

65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for the cookie-delete-throws branch (lines 52-56 in impersonation.ts).

This test verifies cookieStore.delete is called when session isn't admin, but doesn't exercise the catch path where delete() throws (the real-world RSC scenario per Next.js docs, since .set/.delete are only supported in Server Functions/Route Handlers). Add a case where cookieStore.delete throws to confirm getImpersonation still resolves to null without propagating the error.

Suggested additional test

+  it('returns null even if clearing the cookie throws (read-only RSC context)', async () => {
+    cookieStore.value = 'cus_target'
+    headerStore.value = '1'
+    cookieStore.delete.mockImplementation(() => {
+      throw new Error('Cookies can only be modified in a Server Action or Route Handler')
+    })
+    getSessionCustomer.mockResolvedValue({ email: 'attacker@evil.com' })
+    await expect(getImpersonation()).resolves.toBeNull()
+  })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/impersonation.test.ts` around lines 65 - 71, Add test coverage for
the error-handling path in getImpersonation where cookieStore.delete throws in
the non-admin branch. Extend the existing impersonation tests to mock
cookieStore.delete throwing, then verify getImpersonation still returns null and
does not propagate the exception, using the getImpersonation function and
cookieStore delete mock to locate the behavior.

src/lib/impersonation.ts (1)

70-75: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Consider defense-in-depth: verify admin status inside startImpersonation too.

The function trusts the doc comment's precondition ("already verified the caller is an admin") without re-checking. If a future caller invokes this without that check, there's no internal guard against a non-admin session setting the cookie.

♻️ Optional defensive check

+import { getSessionCustomer } from '`@/lib/medusa-auth`'
+import { isAdmin } from '`@/lib/admin`'
+
 export async function startImpersonation(targetId: string): Promise<void> {
+  const session = await getSessionCustomer()
+  if (!isAdmin(session?.email)) throw new Error('Not authorized to impersonate')
   const cookieStore = await cookies()
   cookieStore.set(IMPERSONATION_COOKIE, targetId, IMPERSONATION_COOKIE_OPTIONS)
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/impersonation.ts` around lines 70 - 75, `startImpersonation`
currently relies only on the caller’s precondition and can set
`IMPERSONATION_COOKIE` without verifying the session is an admin. Add an
internal admin check at the start of `startImpersonation` in
`src/lib/impersonation.ts` before calling `cookies()` and setting the cookie,
using the existing auth/admin validation path already used by the admin action
so the function is safe even if called directly.

src/lib/customer-orders.ts (1)

188-203: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid paging the full impersonation order list here.
fetchImpersonatedOrders() goes through listAdminCustomerOrders, which can walk up to 5,000 orders to find one match. The admin /admin/orders endpoint accepts id, so this path can use a single filtered lookup instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/customer-orders.ts` around lines 188 - 203, The getOrderById path is
unnecessarily loading the full impersonation order list through
fetchImpersonatedOrders/listAdminCustomerOrders. Update getOrderById to use a
single filtered admin orders lookup when impersonated access is active, passing
the orderId through the existing fetch/admin order retrieval flow instead of
scanning all impersonated orders. Keep the fallback behavior with getAuthToken
and fetchOrders unchanged, and use the existing getOrderById,
fetchImpersonatedOrders, and fetchOrders symbols to locate the logic.

src/app/api/account/licenses/[licenseId]/discord/members/[memberId]/route.test.ts (1)

11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Guard mock added, but the 403 branch itself is untested here.

This mock only keeps existing tests passing; no test asserts DELETE returns 403 read_only_inspection when assertViewOnly rejects with ViewOnlyError (unlike src/app/api/account/profile/route.test.ts, which added an explicit case for this). Consider adding an analogous test, e.g. mocking assertViewOnly to reject once and asserting the response status/body.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/app/api/account/licenses/`[licenseId]/discord/members/[memberId]/route.test.ts
around lines 11 - 15, The current `route.test.ts` only mocks `assertViewOnly` in
`@/lib/impersonation`, but it does not cover the `DELETE` 403
`read_only_inspection` branch. Add an explicit test for the `DELETE` handler in
this route that makes `assertViewOnly` reject with `ViewOnlyError`, then assert
the response status and body match the expected 403 `read_only_inspection`
behavior, similar to the pattern used in `profile/route.test.ts`.

src/app/[locale]/account/admin/actions.test.ts (1)

1-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid coverage of forbidden/not_found/success paths.

Consider adding a case for the rate_limited branch (Line 40 in actions.ts) since it's currently untested.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`[locale]/account/admin/actions.test.ts around lines 1 - 57, Add test
coverage for the rate-limited path in startImpersonationAction: mock the rate
limiter so consume() returns a failed result, then assert the action returns the
rate_limited error and does not call findAdminCustomerByEmail or
startImpersonation. Keep the new test alongside the existing
startImpersonationAction cases in actions.test.ts and use the existing mocked
createRateLimiter/clientIp setup to target this branch.

src/app/[locale]/account/admin/page.tsx (1)

13-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Result of startImpersonationAction is silently discarded.

On forbidden/not_found/rate_limited, the form just re-renders with no feedback — the admin can't tell why "View as" did nothing. The code comment already flags this as a known follow-up (useActionState).

Want me to wire this page up with useActionState to surface the error to the admin now?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`[locale]/account/admin/page.tsx around lines 13 - 19, The submit
flow in the admin account page is discarding the result from
startImpersonationAction, so forbidden/not_found/rate_limited cases re-render
without any admin-visible feedback. Update the submit/use server flow in
page.tsx to use useActionState (or equivalent action state handling) so the
returned error is stored and rendered inline, and keep the existing
redirect-on-success behavior from startImpersonationAction.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/app/api/account/impersonate/exit/route.ts (2)

7-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider reusing next-intl's getPathname instead of hand-rolled prefixing.

The locale !== defaultLocale ? '/${locale}/account' : '/account' logic duplicates next-intl's own localePrefix: 'as-needed' behavior outside the shared routing config. If that config ever changes (e.g. to always or domain-based routing), this manual branch will silently drift. getPathname({ locale, href: '/account' }) from @/i18n/navigation encodes the same intent without duplicating the prefixing rule.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/account/impersonate/exit/route.ts` around lines 7 - 25, The
accountUrl helper is duplicating locale prefixing logic instead of using the
shared next-intl routing API. Update accountUrl in the impersonate exit route to
build the /account target through getPathname from `@/i18n/navigation`, using the
resolved locale and keeping the existing referer/explicitLocale selection logic,
so prefix behavior stays aligned with the central routing config.

---

27-32: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Same PII-in-logs concern as actions.ts.

authLogger.info\Impersonation STOP: admin=${session?.email ?? 'unknown'}`` logs the admin's email; flagged by static analysis (CWE-532). Lower risk than logging the target customer's email (this is the admin's own identity for audit purposes), but consider standardizing on a non-PII identifier if the same convention is applied elsewhere.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/account/impersonate/exit/route.ts` around lines 27 - 32, The exit
impersonation flow in exit() is still logging PII by using session?.email in
authLogger.info, which triggers the same CWE-532 concern as actions.ts. Update
the log statement in exit to avoid raw email addresses and standardize on a
non-PII identifier already available from getSessionCustomer() or another stable
audit-safe value, keeping the Impersonation STOP event usable without exposing
email data.

Source: Linters/SAST tools

src/lib/discord/medusa-admin.ts (1)

96-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Inconsistent error handling vs. sibling lookups.

findAdminCustomerByEmail and getAdminCustomerById both catch medusaAdminFetch failures and log via infraLogger.error, returning null. getAdminCustomerOrderById lets failures propagate uncaught. The only current caller (fetchImpersonatedOrderById in src/lib/customer-orders.ts) happens to wrap it in try/catch, but this breaks the just-established convention in this file and is fragile against future callers.

♻️ Suggested fix for consistency

 export async function getAdminCustomerOrderById(
   customerId: string,
   orderId: string
 ): Promise<MedusaOrder | null> {
   const query = new URLSearchParams({
     limit: '1',
     customer_id: customerId,
     id: orderId,
   })
-
-  const page = await medusaAdminFetch<AdminOrdersResponse>(
-    `/admin/orders?${query.toString()}`
-  )
-  const [order] = page.orders ?? []
-  return order?.id === orderId ? order : null
+  try {
+    const page = await medusaAdminFetch<AdminOrdersResponse>(
+      `/admin/orders?${query.toString()}`
+    )
+    const [order] = page.orders ?? []
+    return order?.id === orderId ? order : null
+  } catch (error) {
+    infraLogger.error`Failed to fetch admin order ${orderId} for customer ${customerId}: ${error}`
+    return null
+  }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/discord/medusa-admin.ts` around lines 96 - 111,
getAdminCustomerOrderById is the only lookup here that does not mirror the
try/catch + infraLogger.error + null fallback used by findAdminCustomerByEmail
and getAdminCustomerById. Wrap the medusaAdminFetch call in
getAdminCustomerOrderById with the same error handling pattern, log the failure
through infraLogger.error with enough context to identify the order/customer
lookup, and return null on failure so the behavior stays consistent for future
callers.

src/app/[locale]/account/admin/actions.ts (1)

33-58: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Avoid logging customer email addresses in plaintext.

Static analysis flags lines 39, 51, and 55 for logging PII (CWE-532). target=${email} / target=${target.email} log the impersonated customer's email address on every lookup attempt. Consider logging target.id (or a redacted/hashed form) instead of the raw email to preserve audit value while reducing PII exposure in log aggregators.

🔒 Suggested reduction of logged PII

-    authLogger.info`Impersonation lookup miss: admin=${adminEmail} target=${email}`
+    authLogger.info`Impersonation lookup miss: admin=${adminEmail}`
     return { error: 'not_found' }
   }

-  authLogger.info`Impersonation START: admin=${adminEmail} target=${target.email} (${target.id})`
+  authLogger.info`Impersonation START: admin=${adminEmail} target_id=${target.id}`

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`[locale]/account/admin/actions.ts around lines 33 - 58, The
impersonation audit logs in startImpersonationAction are leaking customer email
PII by writing the target email in plaintext. Update the authLogger.info calls
in the lookup-miss and START paths to log a non-PII identifier such as
target.id, or a redacted/hashed value, while keeping the adminEmail context for
auditability. Keep the existing flow in startImpersonationAction and only change
the logged target fields so the messages remain useful without exposing raw
customer emails.

Source: Linters/SAST tools